Skip to content

fix(policies): start a policy's handlers before its new inputs - #801

Merged
leoparente merged 2 commits into
developfrom
fix/policy-handlers-start-before-input
Oct 28, 2026
Merged

leoparente merged 2 commits into
developfrom
fix/policy-handlers-start-before-input

Conversation

@leoparente

@leoparente leoparente commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

A policy on a pcap_file tap saw none of its packets in pktvisord. In the orb-agent lab for MON-444, a 140-packet capture applied through the admin API counted 0 in both the net and dns handlers, and nothing was exported.

Cause. PolicyManager::load created and started each new input stream's <stream>-resources policy before it started the user's policy. Starting the resources policy started the input. Policy::start() starts its handlers before its inputs, but by then the input had already run. A pcap_file, and a dnstap_file, is read whole inside start(), so it was consumed before the user's handlers were connected. A live interface lost only its first milliseconds, which is why this went unnoticed.

Fix.

  • Order: every handler on a new input now runs before the input starts.

    • create_resources_policy creates, registers and starts the resources handler, but not the resources policy.
    • Then policy_ptr->start() starts the user's handlers and then the inputs.
    • Then each resources policy is started and registered. Its handler and its input are already running, and every start() returns early when already running.

    Both the user's handlers and the resources handler see the whole file.

  • Rollback: Policy::start() now stops the handlers it started when a later handler or input fails.

    • The new order starts the handlers before a new input can fail.
    • A sequence handler can't be removed while the next one is still connected to it.
    • Without the rollback, a failed load threw "unable to remove, handler has consumers" from the cleanup. The policy's handlers stayed registered, and every retry got 409 "already exists".
  • Tidying: create_resources_policy now returns the unstarted resources policy, and it lost its unused policy_name parameter. Errors are wrapped once by the caller, so messages are unchanged. The resources handler is registered before its policy, which closes a window where a failed handler registration left a registered policy pointing at a destroyed handler.

Tests

  • Good Config pcap_file input feeds the policy's handlers: a pcap tap on tests/fixtures/dns_ipv4_udp.pcap and a net policy.
    • It checks 140 events and the capture's start timestamp, which failed on develop (0 == 140).
    • It also checks that the resources policy runs and that its handler recorded CPU and memory samples, which failed with the first commit of this PR (null != nullptr).
  • Bad Config: exception on input start with sequence modules: a pcap tap with a missing pcap_file and sequence modules [dns, net]. Two loads must both fail with policy [broken_view] failed to start: Cannot open pcap/pcapng file, leaving no policy or handler behind. Without the Policy::start() rollback it failed with "unable to remove, handler has consumers".
  • The existing mock input-start test also checks that the policy is gone and that a retry fails the same way.
  • Suite: all 25 ctest targets pass on macOS (Release).
  • On a rebuilt pktvisord, through the admin API:
    • pcap_file: the policy counts net 140 and dns 140 (70 queries, 70 replies), against 0 on develop. A DELETE and re-POST counts 140 again. The resources policy records cpu_usage and memory_bytes samples.
    • Broken sequence policy, single tap and multi-tap: a clean 500 twice. Nothing is left but the existing policy, and the shared input's consumer count returns to where it was.

Not addressed here (found in review, all older than this change)

  • Dangling policy pointer on a reused input: a load that fails before the policy has started leaves the policy pointer on a reused, running input. Its policy count stays one too high, so that input and its resources policy are never torn down.
  • Lock order: load takes the input map and then the policy map, while remove_policy takes them the other way round, so a concurrent POST and DELETE can deadlock. This change shortens the window: an input's start() no longer runs while the input map lock is held.
  • Narrow catch in load: it catches std::runtime_error only. A std::logic_error, such as PcapPlusPlus's std::invalid_argument, skips the rollback. With this change the leftover handlers are at least stopped.
  • Live capture by interface name: separately, it fails with PcapPlusPlus 25.05. PcapInputStream::start() assigns the iface string to an IPv4Address and an IPv6Address, and 25.05 throws on anything that isn't an address, so iface: eth0 gives "Not a valid IPv4 address: eth0". Seen in the orb-agent develop image on Linux and on macOS builds. It is unrelated to this change and needs its own fix.

🤖 Generated with Claude Code

PolicyManager::load started each new input stream's -resources policy
before the policy itself, and starting the resources policy started the
input. An input that reads all its data as it starts, such as a pcap_file,
was then consumed before the policy's handlers were connected, so the
policy saw nothing: a 140-packet capture counted 0.

The resources policies are now created after the policy has started its
handlers and then its inputs. Policy::start also stops the handlers it
started when a later handler or input fails, because a sequence handler
cannot be removed while the next one is still connected to it; without
that, a failed load left the policy's handlers registered and every retry
got "already exists".

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@nbl-ai-review

nbl-ai-review Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Summary — No new commits since the last review. Correctness and security were re-checked against the current head; no new findings. Net-positive fix.

Both findings from the previous review were declined by the author with concrete reasoning tied to the code (one already marked dismissed; the other newly declined in the PR conversation). Independent re-verification agrees neither is a live bug, so neither is being re-raised.

🤖 AI Code Review · run · prompts: code-review/v1 · models: claude-sonnet-5/medium · context: description · 10 comment(s) · 1 prior AI review(s) · 1/1 thread(s) resolved · usage: $1.84 · 3398k in (96% cached) · 26k out

Previous review · 2026-10-28 · 5f9d5e5

AI Code Review — risk tier: Lite · dimensions run: correctness, security

Advisory only. A human owns the merge decision.

Summary — Net-positive fix: reorders handler/input startup so a policy's handlers are connected before its inputs run, with a rollback path for partial starts, and includes regression tests for both the original bug and the new failure mode. One small gap remains in the new rollback logic for multi-tap policies.

Findings

  • src/Policies.cpp:328 — [Low] [correctness] Policy::start()'s rollback doesn't stop already-started inputs on a multi-tap policy — The new catch block only re-stops _modules that are running(). A policy built from more than one tap can have multiple _input_streams entries; if an earlier input's start() succeeds and a later input's (or module's) start() throws, the catch stops handlers but leaves the already-started input running. Since _running is never set to true before the throw, the caller's policy_ptr->stop() in PolicyManager::load is a no-op (Policy::stop() returns early when !_running), so that input's capture thread (e.g. a live pcap/flow listener) keeps running after the load has failed and all handlers/inputs are removed from the manager maps. Suggest also stopping any _input_streams entries that are running() in the catch block, mirroring the module loop, before rethrowing.

🤖 AI Code Review · run · prompts: code-review/v1 · models: claude-sonnet-5/medium · context: description · 6 comment(s) · 1 prior AI review(s) · 1/1 thread(s) resolved · usage: $1.83 · 3479k in (96% cached) · 23.7k out

Previous review · 2026-10-28 · cd73275

AI Code Review — risk tier: Full · dimensions run: correctness, security

Advisory only. A human owns the merge decision.

Summary — Net-positive fix: reordering policy start (handlers before inputs, resources-policy after) correctly resolves the described pcap_file/dnstap_file race, and the new Policy::start() rollback fixes the sequence-handler cleanup failure. No security-sensitive surface introduced.

Pre-existing / out-of-scope

  • src/Policies.cpp:169 — The new catch block's rollback (for (auto &p : added_resources_policies) module_remove(p)) calls the base PolicyManager::module_remove, which erases the resources Policy without calling input->remove_policy() on it first. If create_resources_policy() succeeds for one tap of a multi-tap policy but throws for another, this leaves a dangling Policy* in the input's _policies vector and an over-counted policies_count(), so the shared input is never torn down. This is the same class of issue the PR description already flags under "Dangling policy pointer on a reused input" — this call was moved unchanged from the prior single try/catch block, not introduced or worsened here. Worth confirming the known-issue tracking covers this resources-policy variant too, since multi-tap policies make the partial-success-then-failure sequence a realistic trigger.

🤖 AI Code Review · run · prompts: code-review/v1 · models: claude-sonnet-5/medium · context: description · 0 comment(s) · 0 prior AI review(s) · usage: $1.37 · 2200k in (95% cached) · 23k out

@leoparente

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-28T17:33:52.535889Z 5f9d5e5 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd73275e42

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/Policies.cpp Resolved
@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

LCOV of commit 5f9d5e5 during Debug Builds #252

  lines......: 85.8% (17970 of 20948 lines)
  functions..: 75.6% (1811 of 2396 functions)
  branches...: no data found

Files changed coverage rate: n/a

Full coverage report

The previous commit started the policy's handlers before its new inputs,
but created the resources policies afterwards, so for a pcap_file or
dnstap_file input the resources handler connected only after the whole
file had been read and recorded no CPU or memory samples. The resources
handlers are now created, registered and started before the policy starts
its inputs, and the resources policies are started and registered once the
inputs run. Registering the handler before the policy also closes the
window where a failed handler registration left a policy pointing at a
destroyed handler.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@leoparente

Copy link
Copy Markdown
Contributor Author

@codex review

@leoparente

Copy link
Copy Markdown
Contributor Author

AI review finding "Rollback of partially-created resources policies leaves a dangling policy pointer on the shared input": won't do here, because it can't happen in this code.

  • A resources policy is only created for an input this load created (added_inputs); a reused input never gets one, so no shared input can hold a resources policy from a failed load.
  • The same catch removes every one of those inputs with input_manager()->module_remove, which checks signal consumers, not the policy list. The input holding the stale pointer is destroyed a few lines later.
  • Nothing reads _policies in between: InputStream only appends to it, erases from it and counts it.

The dangling pointer that does matter, on a reused input when a load fails before the policy has started, is the separate pre-existing issue listed in the description.

@leoparente

Copy link
Copy Markdown
Contributor Author

/ai-review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: 5f9d5e54aa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@leoparente

Copy link
Copy Markdown
Contributor Author

/ai-review

@leoparente

Copy link
Copy Markdown
Contributor Author

AI review finding "Policy::start() rollback doesn't stop already-started inputs on a multi-tap policy": won't do. The inputs it describes are already stopped, and the suggested change would break shared inputs.

  • New inputs are already stopped. Every input this load created is in added_inputs, and load's catch removes each one with InputStreamManager::module_remove, which calls stop() before erasing it (src/InputStreamManager.h:51). A new input that started before a later one failed does not keep running.
  • Shared inputs must keep running. The other inputs a multi-tap policy can hold are reused ones that other policies already run. start() is a no-op on them, and stopping every running _input_streams entry in the rollback would stop them under those policies.
  • Tested. A review run against the rebuilt pktvisord covered this case (a reused running input plus a new pcap_file input that fails): a clean 500, the server stayed healthy, and the shared input kept its consumers.

@leoparente
leoparente marked this pull request as ready for review October 28, 2026 17:41
@leoparente

Copy link
Copy Markdown
Contributor Author

/ai-review

@nbl-ai-review

nbl-ai-review Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

🔁 AI Code Review updated for 5f9d5e54 — see the review · 0 resolved · 0 open (run)

@leoparente
leoparente merged commit 11878a3 into develop Oct 28, 2026
23 checks passed
@leoparente
leoparente deleted the fix/policy-handlers-start-before-input branch October 28, 2026 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants